drivers: crypto: Add initial support for rpi_pico sha256 accelerator #85036

soburi · 2025-02-03T01:49:02Z

Add basic support for RaspberryPi Pico's SHA256 hardware accelerator.

soburi · 2025-02-14T22:22:08Z

Could you take a look if you have a bit of a while?

valeriosetti

The PR looks OK to me. I cannot test it locally because I don't have a board at hand, but at least the test that was extended builds correctly. I only left one minor non-blocking question/comment.

drivers/crypto/crypto_rpi_pico_sha256.c

ThreeEights · 2025-02-17T16:40:23Z

drivers/crypto/CMakeLists.txt

+zephyr_library_sources_ifdef(CONFIG_CRYPTO_SMARTBOND            crypto_smartbond.c)
+zephyr_library_sources_ifdef(CONFIG_CRYPTO_STM32                crypto_stm32.c)
+zephyr_library_sources_ifdef(CONFIG_CRYPTO_TINYCRYPT_SHIM       crypto_tc_shim.c)
+# zephyr-keep-sorted-stop


Sorting - good idea!

nit: I'd prefer it if the sorting is one commit, and the addition is a separate commit. The end result is the same, but it's easier to to see the two orthogonal things going on.

valeriosetti

Thanks for addressing previous comment. However reviewing the PR again I found a couple of new ones that I missed before. Sorry

drivers/crypto/crypto_rpi_pico_sha256.c

soburi · 2025-03-17T20:04:28Z

@ceolin @ThreeEights @ajf58

Could you take a look at it?

ajf58

The change looks functionally good, but I am concerned about the outright copy and paste of BSD code, then relabeling it as Apache 2.

I feel like someone else needs to comment on this.

ajf58 · 2025-03-17T21:23:15Z

drivers/crypto/CMakeLists.txt

+zephyr_library_sources_ifdef(CONFIG_CRYPTO_SMARTBOND            crypto_smartbond.c)
+zephyr_library_sources_ifdef(CONFIG_CRYPTO_STM32                crypto_stm32.c)
+zephyr_library_sources_ifdef(CONFIG_CRYPTO_TINYCRYPT_SHIM       crypto_tc_shim.c)
+# zephyr-keep-sorted-stop


nit: I'd prefer it if the sorting is one commit, and the addition is a separate commit. The end result is the same, but it's easier to to see the two orthogonal things going on.

ajf58 · 2025-03-17T21:53:37Z

drivers/crypto/crypto_rpi_pico_sha256.c

+LOG_MODULE_REGISTER(sha_rpi_pico_sha256, CONFIG_CRYPTO_LOG_LEVEL);
+
+/*
+ * This is based on the pico_sha256 implementation in the Pico-SDK.


Original code is a https://github.com/raspberrypi/pico-sdk/blob/2.1.0/src/rp2_common/pico_sha256/sha256.c , it's 3-Clause BSD (BSD-3-Clause).

I feel like this file has lifted-and-shifted too much for someone to ignore (many lines of code are the same, just formatted to meet the Zephyr Project's coding style).

I'd be happy if this file had a BSD-3-Clause on it. Zephyr must be compatible with this, we link against BSD-3-Clause all over the place. It's not easy t osee examples of that, however.

Alternatively, we could modify the version of the source code in the hal_rpi_pico. This is something I prefer to avoid, but that feels better at ensuring the software licence is upheld.

The codes derived from pico-sdk were separated into crypto_rpi_pico_sha256.h, and the license changed to BSD-3.

nit: I'd prefer it if the sorting is one commit, and the addition is a separate commit. The end result is the same, but it's easier to to see the two orthogonal things going on.

Addressed.

soburi · 2025-10-24T00:39:49Z

@ajf58 @valeriosetti

Please confirm the change request again.

soburi · 2025-10-24T21:44:26Z

@valeriosetti
I'm sorry again. Thank you.

ceolin · 2025-10-24T22:02:45Z

drivers/crypto/crypto_rpi_pico_sha256.c

+	struct crypto_rpi_pico_sha256_data *data = dev->data;
+	k_spinlock_key_t key;
+
+	if (!data->state.locked) {


you have a TOCTOU here and ended up releasing it more than once. I don't know how much of this is a problem because the important lock here seems to be BOOTROM_LOCK_SHA_256. Btw can't you simply inquire whether or not this lock is LOCKED instead of having the spin lock + state.locked ?

I am a little bit puzzled with this state.lock since the lock to the hw is done by the HAL. What would make sense to me here is something to tell when you are in the middle of an operation and you check here to not release the lock

ceolin · 2025-10-24T22:03:51Z

drivers/crypto/crypto_rpi_pico_sha256.c

+	k_spinlock_key_t key;
+	int ret;
+
+	if (data->state.locked) {


Same thing regarding TOCTOU commented in crypto_rpi_pico_sha256_hash_session_free

and here you could possibly just bootrom_try_acquire_lock(BOOTROM_LOCK_SHA_256); and return EBUSY ...

This has been corrected in conjunction with the above fix.

and here you could possibly just bootrom_try_acquire_lock(BOOTROM_LOCK_SHA_256); and return EBUSY ...

Applied. Thank you.

Add `hardware_sha256/include` to `zephyr_include_directories`. Use the header only. Signed-off-by: TOKITA Hiroshi <[email protected]>

Copilot

Pull request overview

This PR adds initial support for the Raspberry Pi Pico (RP2350) SHA-256 hardware accelerator to Zephyr, enabling hardware-accelerated cryptographic hashing operations on the platform.

Implements a new crypto driver for the RP2350's SHA-256 accelerator
Adds device tree bindings and integration with the pico-sdk HAL
Enables the crypto_hash test suite for the rpi_pico2 platform

Reviewed changes

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
drivers/crypto/crypto_rpi_pico_sha256.c	New driver implementation for SHA-256 hardware accelerator with session management and bootrom locking
drivers/crypto/Kconfig.rpi_pico	New Kconfig entry for the SHA-256 driver configuration
drivers/crypto/Kconfig	Adds source for the new rpi_pico Kconfig file
drivers/crypto/CMakeLists.txt	Integrates the new driver source file into the build system
dts/bindings/crypto/raspberrypi,pico-sha256.yaml	Device tree binding definition for the SHA-256 accelerator
dts/vendor/raspberrypi/rpi_pico/rp2350.dtsi	Adds SHA-256 device node to RP2350 device tree
modules/hal_rpi_pico/Kconfig	Adds configuration option for pico-sdk SHA-256 support
modules/hal_rpi_pico/CMakeLists.txt	Integrates pico-sdk SHA-256 sources and headers into build
boards/raspberrypi/rpi_pico2/rpi_pico2_rp2350a_m33.yaml	Marks crypto as a supported feature for the board
tests/crypto/crypto_hash/testcase.yaml	Adds rpi_pico2 to the platform allowlist for crypto_hash tests
tests/crypto/crypto_hash/src/main.c	Adds device compatibility check for raspberrypi_pico_sha256
tests/crypto/crypto_hash/socs/rp2350a_m33.overlay	Test overlay that enables the SHA-256 device

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-03T14:47:29Z

drivers/crypto/CMakeLists.txt

 zephyr_library_sources_ifdef(CONFIG_CRYPTO_NPCX_SHA crypto_npcx_sha.c)
 zephyr_library_sources_ifdef(CONFIG_CRYPTO_NRF_ECB crypto_nrf_ecb.c)
 zephyr_library_sources_ifdef(CONFIG_CRYPTO_NXP_S32_HSE crypto_nxp_s32_hse.c)
+zephyr_library_sources_ifdef(CONFIG_CRYPTO_RPI_PICO_SHA256      crypto_rpi_pico_sha256.c)


Extra whitespace after CONFIG_CRYPTO_RPI_PICO_SHA256. The formatting should be consistent with other lines (single space between the config name and the source file). This line is also within a zephyr-keep-sorted block, so the extra spaces may interfere with automated sorting.

Suggested change

zephyr_library_sources_ifdef(CONFIG_CRYPTO_RPI_PICO_SHA256 crypto_rpi_pico_sha256.c)

zephyr_library_sources_ifdef(CONFIG_CRYPTO_RPI_PICO_SHA256 crypto_rpi_pico_sha256.c)

Copilot · 2025-12-03T14:47:29Z

drivers/crypto/crypto_rpi_pico_sha256.c

+static int crypto_rpi_pico_sha256_hash_handler(struct hash_ctx *ctx, struct hash_pkt *pkt,
+					       bool finish)
+{
+	struct crypto_rpi_pico_sha256_data *data = ctx->device->data;
+
+	if (!data->state.locked) {
+		LOG_ERR("Invalid lock status: unlocked");
+		return -EINVAL;
+	}
+
+	data->state.cache_used = 0;
+	data->state.cache.word = 0;
+	data->state.total_data_size = 0;
+
+	sha256_err_not_ready_clear();
+	sha256_set_bswap(true);
+	sha256_start();
+
+	pico_sha256_update(&data->state, pkt->in_buf, pkt->in_len);
+
+	if (!finish) {
+		LOG_ERR("Multipart hashing not supported yet");
+		return -ENOTSUP;
+	}
+
+	pico_sha256_write_padding(&data->state);
+	sha256_wait_valid_blocking();
+
+	for (uint i = 0; i < 8; i++) {
+		((uint32_t *)pkt->out_buf)[i] = BSWAP_32((uint32_t)sha256_hw->sum[i]);
+	}
+
+	return 0;
+}


The hash_handler function accesses and modifies shared device state (data->state) without holding the mutex, creating a race condition. While begin_session acquires the bootrom lock and sets state.locked, the mutex is released before the actual hash operation occurs. If multiple contexts call hash_compute() concurrently, they could corrupt the shared state fields (cache_used, cache.word, total_data_size) and interfere with each other's hash computations.

The mutex should be held for the entire duration of the hash operation, not just during session begin/free. Consider acquiring the mutex at the start of this function and releasing it before returning, or redesigning the locking strategy to protect the critical section properly.

@copilot
Fixed. please recheck.

Add basic support for RaspberryPi Pico's SHA256 hardware accelerator. Signed-off-by: TOKITA Hiroshi <[email protected]>

Added sha256 accelerator to rpi_pico. Signed-off-by: TOKITA Hiroshi <[email protected]>

Add rpi_pico2 to the crypto_hash test target. Signed-off-by: TOKITA Hiroshi <[email protected]>

sonarqubecloud · 2025-12-03T15:29:21Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Copilot

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-03T21:02:07Z

drivers/crypto/crypto_rpi_pico_sha256.c

+	sha256_err_not_ready_clear();
+	sha256_set_bswap(true);
+	sha256_start();


Hardware initialization (sha256_err_not_ready_clear(), sha256_set_bswap(), sha256_start()) is performed on every hash operation call. This should be moved to hash_begin_session to initialize hardware once per session, not on every hash computation. This also prevents proper multipart hashing support in the future.

Copilot · 2025-12-03T21:02:08Z

drivers/crypto/crypto_rpi_pico_sha256.c

+	sha256_wait_valid_blocking();
+
+	for (uint i = 0; i < 8; i++) {
+		((uint32_t *)pkt->out_buf)[i] = BSWAP_32((uint32_t)sha256_hw->sum[i]);


[nitpick] The explicit cast (uint32_t) before passing to BSWAP_32 is redundant since BSWAP_32 already operates on and returns uint32_t. The cast can be removed for cleaner code: BSWAP_32(sha256_hw->sum[i])

Suggested change

((uint32_t *)pkt->out_buf)[i] = BSWAP_32((uint32_t)sha256_hw->sum[i]);

((uint32_t *)pkt->out_buf)[i] = BSWAP_32(sha256_hw->sum[i]);

Copilot · 2025-12-03T21:02:08Z

dts/bindings/crypto/raspberrypi,pico-sha256.yaml

+
+compatible: "raspberrypi,pico-sha256"
+
+include: base.yaml


The binding should specify that the reg property is required, as the device tree node uses it (at address 0x400f8000). Similar crypto bindings (e.g., espressif,esp32-sha, intel,adsp-sha, ite,it8xxx2-sha) mark this property as required. Add:

properties: reg: required: true

Suggested change

include: base.yaml

include: base.yaml

properties:

reg:

required: true

Copilot · 2025-12-03T21:02:08Z

drivers/crypto/crypto_rpi_pico_sha256.c

+	data->state.cache_used = 0;
+	data->state.cache.word = 0;
+	data->state.total_data_size = 0;


State initialization should not be performed in the hash handler. These fields (cache_used, cache.word, total_data_size) should be initialized in hash_begin_session when the session starts, not at every hash operation. The current placement means multipart hashing can never work correctly even if implemented, as these fields would be reset on each call.

Suggested change

data->state.cache_used = 0;

data->state.cache.word = 0;

data->state.total_data_size = 0;

/* State initialization moved to session start (hash_begin_session) */

Copilot · 2025-12-03T21:02:09Z

drivers/crypto/crypto_rpi_pico_sha256.c

+	pico_sha256_write_padding(&data->state);
+	sha256_wait_valid_blocking();
+
+	for (uint i = 0; i < 8; i++) {


The loop variable i should be declared as uint32_t instead of uint for consistency with Zephyr coding standards and to avoid potential portability issues. The uint type is not a standard C type.

Suggested change

for (uint i = 0; i < 8; i++) {

for (uint32_t i = 0; i < 8; i++) {

soburi marked this pull request as draft February 3, 2025 01:49

zephyrbot added area: Crypto / RNG platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) labels Feb 3, 2025

zephyrbot requested review from ThreeEights, ceolin and yonsch February 3, 2025 01:49

zephyrbot assigned ceolin Feb 3, 2025

soburi mentioned this pull request Feb 3, 2025

TaskList for Supporting RaspberryPi Pico peripherals #53810

Open

57 tasks

soburi force-pushed the rpi_pico_sha256 branch from 0d6e6b8 to a2b3bb7 Compare February 3, 2025 02:32

soburi added the DNM This PR should not be merged (Do Not Merge) label Feb 3, 2025

soburi marked this pull request as ready for review February 3, 2025 04:02

valeriosetti self-requested a review February 10, 2025 08:31

soburi force-pushed the rpi_pico_sha256 branch from a2b3bb7 to 19c04b7 Compare February 11, 2025 20:31

soburi removed the DNM This PR should not be merged (Do Not Merge) label Feb 11, 2025

valeriosetti reviewed Feb 17, 2025

View reviewed changes

drivers/crypto/crypto_rpi_pico_sha256.c Outdated Show resolved Hide resolved

ThreeEights reviewed Feb 17, 2025

View reviewed changes

valeriosetti mentioned this pull request Feb 19, 2025

Add crypto support to TI cc23x0 SoC #84523

Merged

soburi force-pushed the rpi_pico_sha256 branch from 19c04b7 to 3f8ef55 Compare March 8, 2025 01:59

zephyrbot requested a review from ajf58 March 8, 2025 02:00

soburi requested review from ThreeEights and valeriosetti March 8, 2025 03:34

valeriosetti requested changes Mar 10, 2025

View reviewed changes

drivers/crypto/crypto_rpi_pico_sha256.c Outdated Show resolved Hide resolved

drivers/crypto/crypto_rpi_pico_sha256.c Outdated Show resolved Hide resolved

soburi force-pushed the rpi_pico_sha256 branch 2 times, most recently from a6aeaeb to f2ca146 Compare March 10, 2025 22:42

soburi requested a review from valeriosetti March 11, 2025 05:36

soburi force-pushed the rpi_pico_sha256 branch from f2ca146 to ee650de Compare March 14, 2025 13:15

valeriosetti previously approved these changes Mar 14, 2025

View reviewed changes

ajf58 reviewed Mar 17, 2025

View reviewed changes

valeriosetti previously approved these changes Oct 24, 2025

View reviewed changes

soburi dismissed valeriosetti’s stale review via 9b33cfb October 24, 2025 21:28

soburi force-pushed the rpi_pico_sha256 branch from 639ed1c to 9b33cfb Compare October 24, 2025 21:28

valeriosetti previously approved these changes Oct 24, 2025

View reviewed changes

soburi dismissed valeriosetti’s stale review via 47d0a5c October 24, 2025 21:39

soburi force-pushed the rpi_pico_sha256 branch from 9b33cfb to 47d0a5c Compare October 24, 2025 21:39

valeriosetti previously approved these changes Oct 24, 2025

View reviewed changes

ceolin reviewed Oct 24, 2025

View reviewed changes

soburi dismissed valeriosetti’s stale review via e659f78 November 15, 2025 10:53

soburi force-pushed the rpi_pico_sha256 branch from 47d0a5c to e659f78 Compare November 15, 2025 10:53

soburi force-pushed the rpi_pico_sha256 branch 2 times, most recently from 5a00a54 to 9aeb436 Compare November 30, 2025 11:55

modules: hal_rpi_pico: Add hardware_sha256 to include directories

7283565

Add `hardware_sha256/include` to `zephyr_include_directories`. Use the header only. Signed-off-by: TOKITA Hiroshi <[email protected]>

Copilot AI review requested due to automatic review settings December 3, 2025 14:43

soburi force-pushed the rpi_pico_sha256 branch from 9aeb436 to a84e730 Compare December 3, 2025 14:43

Copilot started reviewing on behalf of soburi December 3, 2025 14:44 View session

Copilot finished reviewing on behalf of soburi December 3, 2025 14:46

Copilot AI reviewed Dec 3, 2025

View reviewed changes

soburi added 3 commits December 4, 2025 00:02

drivers: crypto: Add initial support for rpi_pico sha256 accelerator

a6af0fe

Add basic support for RaspberryPi Pico's SHA256 hardware accelerator. Signed-off-by: TOKITA Hiroshi <[email protected]>

boards: raspberrypi: rpi_pico2: Add sha256 accelerator

6fe2e89

Added sha256 accelerator to rpi_pico. Signed-off-by: TOKITA Hiroshi <[email protected]>

tests: crypto: crypto_hash: Add rpi_pico2 to test target

00e0e5d

Add rpi_pico2 to the crypto_hash test target. Signed-off-by: TOKITA Hiroshi <[email protected]>

soburi force-pushed the rpi_pico_sha256 branch from a84e730 to 00e0e5d Compare December 3, 2025 15:02

soburi requested a review from Copilot December 3, 2025 20:56

Copilot started reviewing on behalf of soburi December 3, 2025 20:56 View session

Copilot finished reviewing on behalf of soburi December 3, 2025 20:59

Copilot AI reviewed Dec 3, 2025

View reviewed changes

	zephyr_library_sources_ifdef(CONFIG_CRYPTO_RPI_PICO_SHA256 crypto_rpi_pico_sha256.c)
	zephyr_library_sources_ifdef(CONFIG_CRYPTO_RPI_PICO_SHA256 crypto_rpi_pico_sha256.c)

	((uint32_t *)pkt->out_buf)[i] = BSWAP_32((uint32_t)sha256_hw->sum[i]);
	((uint32_t *)pkt->out_buf)[i] = BSWAP_32(sha256_hw->sum[i]);

	for (uint i = 0; i < 8; i++) {
	for (uint32_t i = 0; i < 8; i++) {

drivers: crypto: Add initial support for rpi_pico sha256 accelerator #85036

Are you sure you want to change the base?

drivers: crypto: Add initial support for rpi_pico sha256 accelerator #85036

Conversation

soburi commented Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soburi commented Feb 14, 2025

Uh oh!

valeriosetti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valeriosetti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

soburi commented Mar 17, 2025

Uh oh!

ajf58 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soburi commented Oct 24, 2025

Uh oh!

soburi commented Oct 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Dec 3, 2025

Quality Gate passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

soburi commented Feb 3, 2025 •

edited

Loading